Feature/myst 708 kill switch #330

zolia · 2018-09-07T08:43:47Z

tried to find middle ground between @Waldz and @tadovas remarks.

tadovas

ConnectOptions as jsonstructure has meaning in endpoints only. Connection manager should at least define own "feature" options, and openvpn client config should define only fine grained methodds like setConnectRetry(infinte). Don't trash the same dto through the system

tadovas · 2018-09-07T10:49:25Z

core/connection/interface.go

@@ -19,6 +19,7 @@ package connection

 import (
 	"github.com/mysteriumnetwork/node/communication"
+	dto2 "github.com/mysteriumnetwork/node/core/node/dto"


avoid generic packages like DTO

I'd like dto69 more :)

tadovas · 2018-09-07T10:50:21Z

core/connection/interface.go

@@ -31,12 +32,12 @@ type DialogCreator func(consumerID, providerID identity.Identity, contact dto.Co

 // VpnClientCreator creates new vpn client by given session,
 // consumer identity, provider identity and uses state callback to report state changes
-type VpnClientCreator func(session.SessionDto, identity.Identity, identity.Identity, state.Callback) (openvpn.Process, error)
+type VpnClientCreator func(session.SessionDto, identity.Identity, identity.Identity, state.Callback, dto2.ConnectOptions) (openvpn.Process, error)


Connection manager should not depend on external Connect options. At least define them in the same package

tadovas · 2018-09-07T10:52:34Z

openvpn/client_config.go

@@ -26,6 +27,13 @@ type ClientConfig struct {
 	*config.GenericConfig
 }

+// SetConnectOptions sets connect options provided through endpoint parameters
+func (c *ClientConfig) SetConnectOptions(options dto.ConnectOptions) {


Client config should have clear methods -> RestrictReconnects and so on, dont add external dependency as ConnectOptions

Waldz · 2018-09-07T09:56:50Z

cmd/commands/cli/command.go

@@ -23,6 +23,8 @@ import (
 	"log"
 	"strings"

+	"strconv"


Why 2 separations?

Waldz · 2018-09-07T11:22:40Z

core/connection/interface.go

@@ -19,6 +19,7 @@ package connection

 import (
 	"github.com/mysteriumnetwork/node/communication"
+	dto2 "github.com/mysteriumnetwork/node/core/node/dto"


I'd like dto69 more :)

Waldz · 2018-09-07T11:24:27Z

core/connection/manager.go

@@ -71,7 +72,7 @@ func NewManager(mysteriumClient server.Client, dialogCreator DialogCreator,
 	}
 }

-func (manager *connectionManager) Connect(consumerID, providerID identity.Identity) (err error) {
+func (manager *connectionManager) Connect(consumerID, providerID identity.Identity, options dto2.ConnectOptions) (err error) {


+100 for DTO, I am doing same wiith serviceOptions for vpn server

Waldz · 2018-09-07T11:25:11Z

core/node/dto/connect_options.go

+
+// ConnectOptions holds tequilapi connect options
+// swagger:model ConnectOptionsDTO
+type ConnectOptions struct {


Lets have this DTO in connection package

Waldz · 2018-09-07T11:25:46Z

e2e/wallet_helper.go

@@ -35,6 +32,9 @@ import (
 	"github.com/ethereum/go-ethereum/ethclient"
 	"github.com/ethereum/go-ethereum/params"
 	"github.com/mysteriumnetwork/node/tequilapi/client"
+	"github.com/mysteriumnetwork/payments/cli/helpers"
+	mysttoken "github.com/mysteriumnetwork/payments/mysttoken/generated"


+1 good idea

Waldz · 2018-09-07T11:27:08Z

openvpn/client_config.go


 	err := NewDefaultValidator().IsValid(vpnConfig)
 	if err != nil {
 		return nil, err
 	}

 	clientFileConfig := newClientConfig(runtimeDir, configDir)
+	clientFileConfig.SetConnectOptions(options)


You should map ConnectionOptions -> openvpn.config here

Waldz · 2018-09-07T11:27:28Z

tequilapi/client/client.go

@@ -100,13 +102,17 @@ func (client *Client) RegistrationStatus(address string) (RegistrationStatusDTO,
 }

 // Connect initiates a new connection to a host identified by providerID
-func (client *Client) Connect(consumerID, providerID string) (status StatusDTO, err error) {
+func (client *Client) Connect(consumerID, providerID string, disableKill bool) (status StatusDTO, err error) {


Lets have pass all dto.ConnectOptions as argument

Waldz · 2018-09-07T11:34:16Z

tequilapi/endpoints/connection.go

+
+	// connect options
+	// required: false
+	ConnectOptions dto.ConnectOptions `json:"connectOptions,omitempty"`


It's dont have to repeat connectOptions in connectionRequest. IMHO options would be enought in REST contract

Waldz · 2018-09-07T11:53:31Z

core/connection/interface.go


 // Manager interface provides methods to manage connection
 type Manager interface {
 	// Connect creates new connection from given consumer to provider, reports error if connection already exists
-	Connect(consumerID identity.Identity, providerID identity.Identity) error
+	Connect(consumerID identity.Identity, providerID identity.Identity, options dto2.ConnectOptions) error


+100 for separating openvopn specific things to separate struct

Waldz · 2018-09-07T11:54:08Z

tequilapi/client/client.go

-		ProviderID string `json:"providerId"`
+		Identity       string             `json:"consumerId"`
+		ProviderID     string             `json:"providerId"`
+		ConnectOptions dto.ConnectOptions `json:"connectOptions"`


It's dont have to repeat connectOptions in connectionRequest. IMHO options would be enought in REST contract

Waldz · 2018-09-08T08:18:46Z

I imagine transformation between ConnectOption to ClientConfig:

clientConfig.setConnectRetry(times)
clientConfig.setConnectRetryInfinite()

zolia · 2018-09-10T09:31:35Z

Branch rebased on master

tadovas

LGTMing this now. But for future reference:
Connection manager should be responsible for tracking service consuming session (promises etc.) and managing underlying payable service. Therefore service abstraction is needed where concrete service will provide and manage all features expected by consumer. I.e. firewall style traffic blocking in case of VPN fails is only needed for Layer3 tunneling. Proxy style service already has it - if proxy conneciton is lost and proxy is set in system settings - no connection will go out by default.

zolia requested review from tadovas and Waldz as code owners September 7, 2018 08:43

tadovas suggested changes Sep 7, 2018

View reviewed changes

Waldz requested changes Sep 7, 2018

View reviewed changes

zolia force-pushed the feature/MYST-708-kill-switch branch 3 times, most recently from f064527 to ac6af31 Compare September 7, 2018 13:58

zolia force-pushed the feature/MYST-708-kill-switch branch 3 times, most recently from deeed2f to 692f004 Compare September 10, 2018 09:27

zolia force-pushed the feature/MYST-708-kill-switch branch 3 times, most recently from 2c446ef to d098d64 Compare September 11, 2018 10:55

zolia self-assigned this Sep 12, 2018

zolia added 2 commits September 12, 2018 13:55

disable kill switch connect option added

3f31055

disable kill switch for cli

96ff615

zolia force-pushed the feature/MYST-708-kill-switch branch from ae5b821 to 924d7fa Compare September 12, 2018 10:59

kill switch mockup

6d3226f

zolia force-pushed the feature/MYST-708-kill-switch branch from 924d7fa to 6d3226f Compare September 12, 2018 11:27

Waldz approved these changes Sep 12, 2018

View reviewed changes

tadovas approved these changes Sep 13, 2018

View reviewed changes

zolia merged commit 3a306c1 into master Sep 13, 2018

zolia deleted the feature/MYST-708-kill-switch branch September 13, 2018 07:00

zolia mentioned this pull request Sep 13, 2018

service abstraction #360

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/myst 708 kill switch #330

Feature/myst 708 kill switch #330

zolia commented Sep 7, 2018 •

edited

tadovas left a comment

tadovas Sep 7, 2018

Waldz Sep 7, 2018

tadovas Sep 7, 2018

tadovas Sep 7, 2018

Waldz Sep 7, 2018

Waldz Sep 7, 2018

Waldz Sep 7, 2018

Waldz Sep 7, 2018

Waldz Sep 7, 2018

Waldz Sep 7, 2018

Waldz Sep 7, 2018

Waldz Sep 7, 2018

Waldz Sep 7, 2018

Waldz Sep 7, 2018

Waldz commented Sep 8, 2018

zolia commented Sep 10, 2018

tadovas left a comment

Feature/myst 708 kill switch #330

Feature/myst 708 kill switch #330

Conversation

zolia commented Sep 7, 2018 • edited

tadovas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Waldz commented Sep 8, 2018

zolia commented Sep 10, 2018

tadovas left a comment

Choose a reason for hiding this comment

zolia commented Sep 7, 2018 •

edited